Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dock the keyboard hide button to the right side of the screen #480

Merged
merged 13 commits into from
Jan 23, 2019

Conversation

pinarol
Copy link
Contributor

@pinarol pinarol commented Jan 18, 2019

Fixes: #350

This PR docks the keyboard hide button to the right side of the screen. Depends on WordPress/gutenberg#13415

Android:
keyboard1and

iOS:
keyboard1ios

To Test

Testing prerequisites

For WPiOS

Checkout the PRs branch to any arbitrary folder and cd .. to it
run yarn install, yarn start
Open XCode WPiOS on the latest develop
Clean build folder on Xcode, and then run the app

For WPAndroid

open grade.properties at WordPress-Android folder
add wp.BUILD_GUTENBERG_FROM_SOURCE = true to grade.properties
checkout the PRs branch in the subrepo of WordPress-Android repo
cd to WordPress-Android/libs/gutenberg-mobile
run yarn install, yarn start
yarn wpandroid on a separate terminal in the same directory

Test steps

Check with different blocks and verify keyboard hide button is docked to the right side of the screen while other buttons can be scrolled horizontally
Verify also there's no UI anomaly due to #480 (comment)

@pinarol
Copy link
Contributor Author

pinarol commented Jan 18, 2019

@iamthomasbishop could you take a look at this PR and share if you have any comments? Related WPiOS branch is gutenberg/keyboard-hide-button if you'd like to request a build including this.

@iamthomasbishop
Copy link
Contributor

@pinarol Looking good! I have a couple minor notes:

  • We can remove the last divider on the inline tools list
  • I think the sizing of the keyboard-hide button is a little too wide. I believe it should be 44x44, and the icon inside 24x24
  • The icon itself is bumped down 1pt in order to provide better visual alignment – so the margin on this one is 11/10/9/11 (top/right/bottom/left). Note: IIRC, we used a slightly smaller icon here (20x20?), so if that's the case, I would just make sure it's centered horizontally and vertically in the button and then bump it 1pt downward.

Details:

image

@pinarol
Copy link
Contributor Author

pinarol commented Jan 21, 2019

Hi @iamthomasbishop ,
I tried to address your feedbacks, I think it is ready for another look:

keyboard2ios

@iamthomasbishop
Copy link
Contributor

Looks great, @pinarol! :shipit:

This reverts commit f8935b4.
Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@@ -7,13 +7,15 @@ import React from 'react';
import Svg, { G, Rect, Path } from 'react-native-svg';
import { TouchableOpacity } from 'react-native';

import styles from './keyboard-hide-button.scss';

type PropsType = {
onPress: void => void,
};

const KeyboardHideButton = ( props: PropsType ) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we created KeyboardHideButton just as a temp solution for the alpha, maybe it's a good opportunity to remove this and create the keyboard hide button as every other Toolbar button, @koke @pinarol WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds good @marecar3

@pinarol
Copy link
Contributor Author

pinarol commented Jan 23, 2019

Hey @marecar3, this is ready for another look.

@marecar3
Copy link
Contributor

Hey @pinarol
Looks good to me :)

@pinarol pinarol merged commit 5285515 into develop Jan 23, 2019
@pinarol pinarol deleted the issue/350-keyboard-hide-button branch January 23, 2019 15:22
daniloercoli added a commit that referenced this pull request Jan 24, 2019
…rg-mobile into issue/372-add-title-to-gutenberg-mobile

* 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile:
  Make sure to use `setContent` when initializating the GlueCode
  Dock the keyboard hide button to the right side of the screen (#480)
  RNAztecView: Removing branch spec from podspec
  Moving RNTAztecView.podspec to the gutenberg-mobile root dir.
  Aztec iOS: Force send height information to JS after pasting text

# Conflicts:
#	gutenberg
#	react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java
daniloercoli added a commit that referenced this pull request Jan 25, 2019
…rg-mobile into issue/372-add-enter-key-detection-to-Title-block

* 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile:
  Update bundle
  bump aztec android to v1.3.18 (#498)
  Update GB hash
  [Android] - Make sure mContentChanged has the correct value when asking content from the React side. This was necessary since the call to retrieve content and title are separate on Android.
  The HTML editor background is now white.
  Make sure to use `setContent` when initializating the GlueCode
  Dock the keyboard hide button to the right side of the screen (#480)
  RNAztecView: Removing branch spec from podspec
  Moving RNTAztecView.podspec to the gutenberg-mobile root dir.
  Aztec iOS: Force send height information to JS after pasting text

# Conflicts:
#	gutenberg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants